refactor: incrementally remove Meteor deps from the frontend#40268
Open
refactor: incrementally remove Meteor deps from the frontend#40268
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
Contributor
WalkthroughThis pull request removes Meteor framework dependencies across the codebase, replacing Meteor's startup callbacks, Tracker-based reactivity, ReactiveVar state management, and DDP methods with modern alternatives: store subscriptions, event emitters, React hooks (useSyncExternalStore), exported client functions, and standard web APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
ggazzo
force-pushed
the
worktree-remove-meteor-frontend
branch
from
2d71f41 to
dd3dac4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40268 +/- ##
===========================================
+ Coverage 69.80% 69.84% +0.04%
===========================================
Files 3296 3296
Lines 119173 119231 +58
Branches 21435 21446 +11
===========================================
+ Hits 83183 83277 +94
+ Misses 32684 32649 -35
+ Partials 3306 3305 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
worktree-remove-meteor-frontend
branch
5 times, most recently
from
01d8539 to
16fe4f8
Compare
ggazzo
changed the title
VideoRecorder used meteor/reactive-var for cameraStarted, but the React consumer never subscribed reactively (no useTracker / useSyncExternalStore), so the send button only re-evaluated its disabled state by accident. Drop meteor/reactive-var from this module and restore reactivity using @rocket.chat/emitter — the pattern already used elsewhere in the client — plus a useSyncExternalStore-based useVideoRecorderCameraStarted hook so VideoMessageRecorder re-renders when the camera starts/stops. The unused private recording flag is removed; recordingAvailable stays since it gates the first-data-available transition internally.
Replace Meteor.absoluteUrl(path) with new URL(path, ROOT_URL), resolving the worker script via the standard URL API. Preserves subpath-deployment behavior since __meteor_runtime_config__.ROOT_URL already carries the configured base.
AutoTranslate.init() is idempotent and the module is imported through the client startup chain, so wrapping in Meteor.startup was a no-op in practice.
Meteor.Error was only used to narrow the .error string on a caught result; replace with an inline structural cast.
Meteor._localStorage wraps window.localStorage with a try/catch for browsers that throw on access (Safari private mode, sandboxed iframes). Inline the same wrapper locally so the util no longer needs the Meteor import.
timeAgo is only called from React components, which do not establish a Tracker computation, so Tracker.nonreactive around getUserPreference never has an outer autorun to opt out of — the wrapper is a no-op.
…stic call Meteor.methods registered setReaction as a client-side stub so that Meteor.callAsync would run it locally for optimistic UI. Move that logic into an exported runOptimisticSetReaction function and call it directly from processSetReaction (the sole caller), then REST-call the server method. Drops meteor/meteor from this module and removes the side-effectful barrel that only existed to register the stub.
…stic call Same approach as setReaction: convert the client-side Meteor.methods stub into an exported runOptimisticSendMessage function and invoke it from flows/sendMessage before the sdk.call. The slash-command asciiart callers (/tableflip, /lenny, etc.) no longer get an optimistic insert — those flows now wait on the server roundtrip like the REST /chat.sendMessage callers always did.
This file only exists to point Meteor.absoluteUrl at the page's base URI. Once the webapp/DDP layer is gone, Meteor.absoluteUrl itself goes away and so does this override — leave a TODO to remove them together.
The Tracker.autorun wrapping the inquiries fetch never re-ran in practice: its only dependency, settings.peek, is non-reactive by design, so changes to Livechat_guest_pool_max_number_incoming_livechats_displayed weren't triggering a refetch. Preserve the intended reactivity without Meteor: fetch once on subscribe, then observe the setting via settings.observe and refetch when it changes. The teardown returned by subscribe now unobserves.
…dule
Two Tracker.autorun calls are replaced with direct Zustand/settings
subscriptions:
- The module-level cache of the current user's language/username moves
from Tracker.autorun(watchUser) to plain subscriptions on userIdStore
and the Users store, triggering refreshUserCache on change.
- init() no longer uses Tracker. It subscribes to userIdStore,
settings.observe('AutoTranslate_Enabled'), and
PermissionsCachedStore.useReady, and retries tryLoad on any of those
signals until the gate (logged in + setting on + permission granted)
is passed. Once passed, the subscriptions tear down — matching the
original 'computation.stop()' one-shot semantics.
Also drops the Meteor.startup wrapper: importPackages loads this module
after startup has already fired, so it ran synchronously anyway.
…ter + hook Per-room history state (hasMore, hasMoreNext, isLoading, unreadNotLoaded, firstUnread) moves from ReactiveVar wrappers to plain fields on a RoomHistoryState object. The manager emits 'state:<rid>' whenever a field is patched through the new updateRoom method, and exposes a useRoomHistoryState(rid, selector) hook built on useSyncExternalStore. RoomProvider and useUnreadMessages switch from useReactiveValue to the new hook, and readStateManager uses direct reads plus updateRoom for its internal writes. Tracker.nonreactive wrappers become plain property reads, and Tracker.afterFlush in the scroll handler becomes queueMicrotask — no consumer was relying on Tracker flush ordering.
Both useCorsSSLConfig (patches Meteor.absoluteUrl.defaultOptions.secure) and oauthRedirectUri (monkey-patches meteor/oauth for pre-2.3 clients) only exist to bridge Meteor behaviour. They'll be deleted alongside the webapp/DDP layer — leave TODOs pointing to that.
Meteor.Error/Meteor.TypedError were only used to widen the callback error type from loginWithToken. Replace with an inline structural alias so useIframe no longer imports meteor/meteor.
Replace Meteor.absoluteUrl with new URL against __meteor_runtime_config__.ROOT_URL, same treatment as AudioEncoder. Preserves subpath-deployment behaviour.
Accounts.storageLocation is typed as Window['localStorage'] in the
project externals, and the key being read ('e2e.randomPassword') is
not an Accounts-managed key. Use window.localStorage directly.
The Tracker.autorun wrapping the sendMessage call had no reactive dependencies — openedRoom is a closure value from useOpenedRoom, not a reactive source — so the autorun ran exactly once and c.stop() was never reached on mismatch. Replace with a plain conditional, which preserves the actual behaviour. Also add the missing @rocket.chat/random import that was relying on an implicit global.
…vider The Session context only needs a string-keyed store plus per-key subscriptions. Swap Meteor's Session (plus createReactiveSubscriptionFactory, which wrapped Session.get in a Tracker.autorun) for a plain Map backed by @rocket.chat/emitter. Equality check mirrors Meteor's Session.set — no emit when the value hasn't changed — so useSession consumers (useUnread, AuthenticationCheck, SidebarToggler, etc.) re-render only on actual writes. This was the last client-side import of meteor/session.
Inline LoginWithExternalServiceOptions locally (same shape as @types/meteor defines) so the interface file no longer imports meteor/meteor. The type is exported from this module for any future consumer that wants to share it; existing OAuth providers still reference Meteor.LoginWithExternalServiceOptions directly and will be migrated alongside the rest of the Accounts layer.
The Tracker.autorun wrapped a setTimeout body that reads no reactive state and immediately calls c.stop(). Replace with a plain setTimeout; cleanup is clearTimeout.
The module is loaded through importPackages after Meteor has already started, so the outer Meteor.startup call fires synchronously — it's purely noise. onLoggedIn stays; the actual side effects (two notify-user stream subscriptions) run module-top-level now.
The Tracker.autorun/afterFlush that waited for watchUserId to become truthy is replaced with a userIdStore.subscribe that tears itself down once a uid is seen. onLoggedIn (for the initial roles.list fetch) and the event handlers are unchanged; the outer Meteor.startup wrapper is dropped as a no-op.
The Tracker.autorun gated the autotranslate streamMessage handler on
AutoTranslate_Enabled (reactive via settings.watch) and hasPermission
('auto-translate') — the latter reactive through the watch() helper's
Tracker reads of Users/Permissions.
Replace with direct subscriptions on settings.observe, PermissionsCachedStore.useReady
and Users.use that re-run applyAutoTranslateStreamHandler. Same
toggle-on/toggle-off semantics, no Tracker dependency.
ggazzo
force-pushed
the
worktree-remove-meteor-frontend
branch
3 times, most recently
from
af56736 to
79dc660
Compare
…th DDP The Tracker.autorun here bridges Meteor's Accounts.connection.userId to the Zustand userIdStore. Replacing it with Accounts.onLogin/onLogout was attempted but breaks callers (UserProvider's logoutCleanUp) that read userIdStore from within other onLogout callbacks and rely on the async Tracker.flush ordering to still see the authenticated uid. Keep the autorun — it'll be deleted together with the Accounts/DDP layer.
The original Tracker.autorun gated user-data sync on watchUserId() + Meteor.status().connected + !Meteor.loggingIn() before fetching. onLoggedIn fires after login completes, when the connection is necessarily up and loggingIn is false, so those two Meteor-specific gates become redundant. Use onLoggedIn for the initial sync + utcOffset update, subscribe to the Users store to emit 'status-changed' on subsequent server-side status changes (delivered via the user stream), and Accounts.onLogout to reset local state. No Tracker, no Meteor.status, no Meteor.loggingIn.
businessHourManager holds a single behavior instance with no reactive store under it, so the Tracker.autorun inside useReactiveValue never re-fired. Inline the plain read.
useMessageComposerIsReadOnly, useFileUploadDropTarget, and ReactionMessageAction
all wrapped a roomCoordinator.readOnly(room, user) call in useReactiveValue
to pick up changes in the post-readonly permission.
Replace with usePermission('post-readonly', room._id) as an explicit
dependency of a plain useMemo — when the permission flips, the memo
re-runs and roomCoordinator.readOnly returns the up-to-date value
(hasPermission underneath is Tracker-aware but returns the current
value when no autorun is active).
The canSendMessage room directive reads Subscriptions.state.count, which is a Zustand snapshot — Tracker.autorun inside useReactiveValue never registered a dep on it, so the previous reactivity was effectively broken. Subscribe properly via useSyncExternalStore on Subscriptions.use so canSend updates when the user joins or leaves the room.
ggazzo
force-pushed
the
worktree-remove-meteor-frontend
branch
from
79dc660 to
01525f2
Compare
Member
Author
|
/jira ARCH-2118 |
Member
Author
|
/jira ARCH-2116 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx (1)
27-44:⚠️ Potential issue | 🔴 CriticalCritical:
sendMessagewill never fire due to staleopenedRoomclosure.
roomCoordinator.openRouteLink()invokes asynchronous navigation (router.navigate()). The room opening—driven byRoomProvider'suseEffect(() => { RoomManager.open(rid) }, [rid])— completes only after the route changes asynchronously. At line 37,openedRoomis a closed-over snapshot from the component's render (captured before the modal was opened), so the conditionopenedRoom === result.ridwill never be true.The prior
Tracker.autorun()was intentionally reactive: it polledopenedRoomreactively and fired the message once the room actually opened. The synchronous replacement bypassed this wait entirely.Since
createPrivateGrouphas already succeeded and the modal is routing directly to that room, just send the message unconditionally:Suggested fix
- const openedRoom = useOpenedRoom(); - const sendInvite = async () => { const privateGroupName = `${name.replace(/\s/g, '-')}-${Random.id(10)}`; try { const result = await callWithErrorHandling('createPrivateGroup' as any, privateGroupName, users); roomCoordinator.openRouteLink(result.t, result); - if (openedRoom === result.rid) { - callWithErrorHandling('sendMessage', { - _id: Random.id(), - rid: result.rid, - msg: t('Apps_Game_Center_Play_Game_Together', { name }), - }); - } + callWithErrorHandling('sendMessage', { + _id: Random.id(), + rid: result.rid, + msg: t('Apps_Game_Center_Play_Game_Together', { name }), + }); onClose(); } catch (err) { console.warn(err); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx` around lines 27 - 44, The sendInvite handler relies on a stale closed-over openedRoom value so the conditional `if (openedRoom === result.rid)` prevents sending the message; remove that conditional and call `callWithErrorHandling('sendMessage', { _id: Random.id(), rid: result.rid, msg: t('Apps_Game_Center_Play_Game_Together', { name }) })` unconditionally after `roomCoordinator.openRouteLink(result.t, result)` inside `sendInvite`, keeping `onClose()` as-is; reference functions/identifiers: sendInvite, openedRoom, roomCoordinator.openRouteLink, callWithErrorHandling('sendMessage'), result.rid, Random.id, and t.
🧹 Nitpick comments (8)
apps/meteor/client/views/omnichannel/businessHours/useIsSingleBusinessHours.ts (1)
3-4: Drop the rationale comment.The two-line explanation of why
useReactiveValuewas removed is historical context that belongs in the commit message, not in the source. As per coding guidelines: "Avoid code comments in the implementation".✏️ Proposed fix
import { businessHourManager } from '../../../../app/livechat/client/views/app/business-hours/BusinessHours'; -// businessHourManager holds a single behavior instance with no reactive store — the previous -// useReactiveValue wrapper never actually re-ran, so this is a plain read. export const useIsSingleBusinessHours = () => businessHourManager.getTemplate() === 'livechatBusinessHoursForm';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/businessHours/useIsSingleBusinessHours.ts` around lines 3 - 4, Remove the historical rationale comment above the businessHourManager reference in useIsSingleBusinessHours.ts: delete the two-line comment starting with "businessHourManager holds a single behavior instance..." and its follow-up about useReactiveValue so the file only contains implementation-relevant comments; leave any necessary code comments intact and do not add new explanatory history (commit message should hold the context).apps/meteor/client/views/root/hooks/useIframe.ts (1)
5-5: Optional:CallbackError's extra fields are unused here.The handler only checks truthiness of
error; it never readserror.error,error.reason, orerror.details. You could simplify the parameter type toError | null | undefinedand drop theCallbackErroralias, or keep the alias and actually surface those fields (e.g., pass richer context viacallback). Non-blocking.Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/root/hooks/useIframe.ts` at line 5, The CallbackError alias declared as "type CallbackError = Error & { error?: string | number; reason?: string; details?: unknown }" is unnecessary because the handler only checks truthiness of the error; either remove the alias and use "Error | null | undefined" in the callback signatures, or keep the alias but update any call sites to pass the extra fields into the callback so they are consumed; update the relevant function/method signatures (the callback parameter types in useIframe's handlers where CallbackError is referenced) to the chosen simpler type or ensure code constructs and forwards the richer object so the extra properties are used.apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
359-363: Optional: document/guard selector stability inuseRoomHistoryState.Because
updateRoommutates the room object in place andgetRoomreturns the same reference,useSyncExternalStorerelies on the selector returning a referentially stable value. All current callers return primitives, so this is safe today — but a future caller that returns a new object/array from the selector (e.g.,s => ({ hasMore: s.hasMore, isLoading: s.isLoading })) would trigger React's "getSnapshot should be cached" warning and an update loop.Consider a brief JSDoc on the export noting the selector-must-return-stable-value contract, or accept a third
isEqualarg to be forwarded viauseSyncExternalStoreWithSelectorfor object selectors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts` around lines 359 - 363, useRoomHistoryState uses useSyncExternalStore with RoomHistoryManager.getRoom which returns the same mutated room reference via updateRoom, so selectors must return referentially stable values; document this contract or make the hook accept an optional equality function and switch to useSyncExternalStoreWithSelector to prevent "getSnapshot should be cached" loops. Specifically, update the export useRoomHistoryState (and note RoomHistoryManager.getRoom / updateRoom) by either adding a JSDoc comment that selectors must return primitives or stable references, or change the signature to useRoomHistoryState<T>(rid, selector, isEqual?) and forward isEqual to useSyncExternalStoreWithSelector so callers can pass a deep-equality for object/array selectors.apps/meteor/client/startup/streamMessage/autotranslate.ts (2)
18-22: Minor race in the dynamic-import.then().If
applyAutoTranslateStreamHandleris invoked multiple times in quick succession whileisEnabledflips, the pendingimport().then()blocks resolve in order but each one unconditionally re-adds the handler. The final registration will reflect the last enabled call, but briefly the callback list churns. A small guard comparing the current enabled state inside the.then()avoids re-adding when disablement raced in:♻️ Example
void import('../../../app/autotranslate/client').then(({ createAutoTranslateMessageStreamHandler }) => { + if (!settings.peek('AutoTranslate_Enabled') || !hasPermission('auto-translate')) return; const streamMessage = createAutoTranslateMessageStreamHandler(); clientCallbacks.remove('streamMessage', STREAM_HANDLER_ID); clientCallbacks.add('streamMessage', streamMessage, clientCallbacks.priority.HIGH - 3, STREAM_HANDLER_ID); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/startup/streamMessage/autotranslate.ts` around lines 18 - 22, The dynamic import's .then() unconditionally re-adds the handler and can race with isEnabled flipping; update the resolved callback to check the current enabled state before re-adding: after calling createAutoTranslateMessageStreamHandler(), call the same isEnabled check (or the current enable flag) and only invoke clientCallbacks.add('streamMessage', streamMessage, ..., STREAM_HANDLER_ID) if it's still enabled, otherwise ensure clientCallbacks.remove('streamMessage', STREAM_HANDLER_ID) and skip adding. Reference createAutoTranslateMessageStreamHandler, clientCallbacks.add/remove, and STREAM_HANDLER_ID in the change.
25-28: Consider narrowing the re-run triggers.
Users.use.subscribe(applyAutoTranslateStreamHandler)on line 28 fires on every user record mutation in the store (not just the current user), and each fire re-enters the asyncimport()chain and rotates thestreamMessagecallback registration. Since the enablement check only readssettings.peek('AutoTranslate_Enabled')andhasPermission('auto-translate'), a userId-scoped trigger (or none at all, if permission changes already come throughPermissionsCachedStore.useReady) would be sufficient.♻️ Example
-import { Users } from '../../stores'; +import { userIdStore } from '../../lib/user'; ... applyAutoTranslateStreamHandler(); settings.observe('AutoTranslate_Enabled', applyAutoTranslateStreamHandler); PermissionsCachedStore.useReady.subscribe(applyAutoTranslateStreamHandler); -Users.use.subscribe(applyAutoTranslateStreamHandler); +userIdStore.subscribe(applyAutoTranslateStreamHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/startup/streamMessage/autotranslate.ts` around lines 25 - 28, The Users.use.subscribe(applyAutoTranslateStreamHandler) subscription is too broad and re-runs the async import/handler rotation on every user record change; narrow it by subscribing only to the current user's changes or remove it if permission changes are already covered by PermissionsCachedStore.useReady. Update the registration around applyAutoTranslateStreamHandler so it either replaces Users.use.subscribe(...) with a userId-scoped watcher (e.g., subscribe/observe for the current user's record or Users.findOne(currentUserId).observe/subscribe) or delete that line and rely on settings.observe('AutoTranslate_Enabled') and PermissionsCachedStore.useReady; keep the enablement checks (settings.peek('AutoTranslate_Enabled') and hasPermission('auto-translate')) unchanged so the handler only re-initializes when the current user’s permissions or the setting actually change.apps/meteor/app/autotranslate/client/lib/autotranslate.ts (1)
19-28: Optional: narrowUsers.use.subscribe(refreshUserCache)to changes of the current user.Every
Users.usemutation (including unrelated users in the store) re-runsrefreshUserCacheand walks aMap.get(uid). Benign, but if you want to avoid the churn, useUsers.use.subscribewith a selector/equality function or early-return when the cacheduserreference is unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/autotranslate/client/lib/autotranslate.ts` around lines 19 - 28, The refreshUserCache function is being run on every Users.use mutation; narrow subscription by either subscribing to just the current user's entry or early-returning when the cached user hasn't changed: replace Users.use.subscribe(refreshUserCache) with a targeted subscribe that selects Users.use.getState().get(uid) (or provide a selector/equality pair) so the handler only runs when that specific user's value changes, or modify refreshUserCache to compare the fetched user reference to a cachedUser variable and return if unchanged; update references to refreshUserCache, Users.use.subscribe, Users.use.getState().get(uid), userLanguage and username accordingly.apps/meteor/app/reactions/client/methods/setReaction.ts (1)
44-65: Avoid mutating the stored message; also redundant guard on line 51.Two small concerns in the removal branch:
messageis the reference returned byMessages.state.get(messageId). Lines 45 / 48 / 52 mutate the stored object (and its nestedusernamesarray) in-place before callingMessages.state.update(...). Any subscriber that holds the same reference or shallow-comparesrecord.reactions(same reference survives the spread on line 62) may miss the change. Build a new reactions object fromrecordinside the updater instead of mutating the fetchedmessage.- Line 51's
!message.reactions || typeof message.reactions !== 'object'is unreachable — line 44 already provedmessage.reactionsis a truthy object, and nothing between 45–49 can turn it into anything else. It reduces toObject.keys(message.reactions).length === 0.♻️ Proposed refactor (non-mutating + simplified guard)
- if (message.reactions?.[reaction] && message.reactions[reaction].usernames.indexOf(user.username) !== -1) { - message.reactions[reaction].usernames.splice(message.reactions[reaction].usernames.indexOf(user.username), 1); - - if (message.reactions[reaction].usernames.length === 0) { - delete message.reactions[reaction]; - } - - if (!message.reactions || typeof message.reactions !== 'object' || Object.keys(message.reactions).length === 0) { - delete message.reactions; - Messages.state.update( - (record) => record._id === messageId, - ({ reactions: _, ...record }) => record, - ); - return; - } - - Messages.state.update( - (record) => record._id === messageId, - (record) => ({ ...record, reactions: message.reactions }), - ); - return; - } + if (message.reactions?.[reaction]?.usernames.includes(user.username)) { + Messages.state.update( + (record) => record._id === messageId, + (record) => { + const current = record.reactions?.[reaction]; + if (!current) return record; + const usernames = current.usernames.filter((u) => u !== user.username); + const { [reaction]: _removed, ...rest } = record.reactions ?? {}; + const nextReactions = usernames.length === 0 ? rest : { ...rest, [reaction]: { ...current, usernames } }; + if (Object.keys(nextReactions).length === 0) { + const { reactions: _r, ...recordWithoutReactions } = record; + return recordWithoutReactions; + } + return { ...record, reactions: nextReactions }; + }, + ); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/client/methods/setReaction.ts` around lines 44 - 65, The removal branch mutates the stored message and contains an unreachable guard; instead, stop modifying the fetched message and perform removal immutably inside the Messages.state.update updater: call Messages.state.update((record) => record._id === messageId, (record) => { build a shallow copy of record.reactions (e.g., {...record.reactions}), copy the usernames array for the target reaction, filter out user.username, if the filtered usernames is empty delete that reaction key, if no reactions remain delete reactions from the copied record, then return the new record }), and remove the redundant `!message.reactions || typeof message.reactions !== 'object'` check since message.reactions was already proven truthy by the initial branch; reference symbols: message, messageId, reaction, user.username, Messages.state.update.apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.ts (1)
13-13: Remove the deadrecordingAvailablefield and its associated writes.The field is only read once in a guard condition (line 89) before being set to
true, and nothing subsequently reads it. The guard at lines 89–91 can be removed entirely, as the assignment is idempotent. Also remove the reset at line 146. The field is unused state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.ts` at line 13, Remove the unused instance field recordingAvailable and all writes to it: delete the private recordingAvailable declaration, remove the guard that checks recordingAvailable before assigning true (the idempotent assignment can remain or be removed), and remove the reset assignment (where recordingAvailable is set back to false). Ensure no other code reads recordingAvailable; if any remaining reference exists, remove it so the class no longer maintains this unused state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/ui/client/lib/recorderjs/AudioEncoder.ts`:
- Line 11: The worker URL construction in AudioEncoder.ts uses new
URL('workers/mp3-encoder/index.js', __meteor_runtime_config__.ROOT_URL) which
breaks when ROOT_URL lacks a trailing slash; update the logic that computes
workerPath to ensure the base (__meteor_runtime_config__.ROOT_URL) ends with a
'/' before passing it to new URL (or otherwise join the path correctly) so that
the worker resolves under the app subpath (e.g., keep last segment from being
replaced); change the assignment that defines workerPath accordingly (the
variable and its use in AudioEncoder.ts).
In `@apps/meteor/client/lib/rooms/roomCoordinator.tsx`:
- Around line 205-211: The permalink construction loses the ROOT_URL sub-path
because router.buildRoutePath() returns an absolute path and new URL(path,
__meteor_runtime_config__.ROOT_URL) replaces the base pathname; update the code
in roomCoordinator.getURL() (where router.buildRoutePath(...) and
__meteor_runtime_config__.ROOT_URL are used) to either call
Meteor.absoluteUrl(router.buildRoutePath(...)) like before, or pass a relative
path to new URL by stripping the leading '/' from router.buildRoutePath(...) so
the base pathname is preserved; ensure getPermaLink() consumers receive the
corrected URL.
In `@apps/meteor/client/startup/incomingMessages.ts`:
- Around line 8-32: The onLoggedIn callbacks should return cleanup functions
that call the subscription's .stop method because sdk.stream(...) returns an
object with a .stop property (not a function), so change both usages to capture
the return value (e.g., const sub = sdk.stream(...)) and return a function that
invokes sub.stop() (or sub.stop?.()) so the stream unsubscribes on
logout/re-login; update the first onLoggedIn that calls Messages.state.store and
the second that sets up subscriptions-changed to follow this pattern.
In `@apps/meteor/client/startup/startup.ts`:
- Around line 43-46: The onLogout handler in startup.ts removes user data
immediately and runs before UserProvider's onLogout, causing Users.state to be
cleared before UserProvider can run afterLogoutCleanUpCallback.run(user) and
sdk.call('logoutCleanUp', user); change the startup onLogout to postpone the
destructive work so component-level handlers run first—e.g. replace the
immediate removeLocalUserData()/status reset with a deferred call (use
Meteor.defer or setTimeout 0) or otherwise register this cleanup after
UserProvider mounts; keep references to Accounts.onLogout in startup.ts,
removeLocalUserData, Users.state.replaceAll, and UserProvider's
afterLogoutCleanUpCallback.run/sdk.call to verify ordering.
- Around line 48-56: The current heuristic in startup.ts uses
(!userIdStore.getState() && localStorage.getItem('Meteor.loginToken') === null)
and calls removeLocalUserData(), but removeLocalUserData() currently clears all
of localStorage which is destructive on fresh visits; update the fix so that
removeLocalUserData() only removes the specific E2EE keys (e.g., 'public_key',
'private_key') instead of calling localStorage.clear(), or alternatively add a
stronger resume-failure signal (e.g., set/check a session-expiry marker key
before clearing) and only remove E2EE keys when that marker indicates an expired
resume; use the existing symbols userIdStore, Meteor.loginToken, and
removeLocalUserData to locate and implement the change.
---
Outside diff comments:
In `@apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx`:
- Around line 27-44: The sendInvite handler relies on a stale closed-over
openedRoom value so the conditional `if (openedRoom === result.rid)` prevents
sending the message; remove that conditional and call
`callWithErrorHandling('sendMessage', { _id: Random.id(), rid: result.rid, msg:
t('Apps_Game_Center_Play_Game_Together', { name }) })` unconditionally after
`roomCoordinator.openRouteLink(result.t, result)` inside `sendInvite`, keeping
`onClose()` as-is; reference functions/identifiers: sendInvite, openedRoom,
roomCoordinator.openRouteLink, callWithErrorHandling('sendMessage'), result.rid,
Random.id, and t.
---
Nitpick comments:
In `@apps/meteor/app/autotranslate/client/lib/autotranslate.ts`:
- Around line 19-28: The refreshUserCache function is being run on every
Users.use mutation; narrow subscription by either subscribing to just the
current user's entry or early-returning when the cached user hasn't changed:
replace Users.use.subscribe(refreshUserCache) with a targeted subscribe that
selects Users.use.getState().get(uid) (or provide a selector/equality pair) so
the handler only runs when that specific user's value changes, or modify
refreshUserCache to compare the fetched user reference to a cachedUser variable
and return if unchanged; update references to refreshUserCache,
Users.use.subscribe, Users.use.getState().get(uid), userLanguage and username
accordingly.
In `@apps/meteor/app/reactions/client/methods/setReaction.ts`:
- Around line 44-65: The removal branch mutates the stored message and contains
an unreachable guard; instead, stop modifying the fetched message and perform
removal immutably inside the Messages.state.update updater: call
Messages.state.update((record) => record._id === messageId, (record) => { build
a shallow copy of record.reactions (e.g., {...record.reactions}), copy the
usernames array for the target reaction, filter out user.username, if the
filtered usernames is empty delete that reaction key, if no reactions remain
delete reactions from the copied record, then return the new record }), and
remove the redundant `!message.reactions || typeof message.reactions !==
'object'` check since message.reactions was already proven truthy by the initial
branch; reference symbols: message, messageId, reaction, user.username,
Messages.state.update.
In `@apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts`:
- Around line 359-363: useRoomHistoryState uses useSyncExternalStore with
RoomHistoryManager.getRoom which returns the same mutated room reference via
updateRoom, so selectors must return referentially stable values; document this
contract or make the hook accept an optional equality function and switch to
useSyncExternalStoreWithSelector to prevent "getSnapshot should be cached"
loops. Specifically, update the export useRoomHistoryState (and note
RoomHistoryManager.getRoom / updateRoom) by either adding a JSDoc comment that
selectors must return primitives or stable references, or change the signature
to useRoomHistoryState<T>(rid, selector, isEqual?) and forward isEqual to
useSyncExternalStoreWithSelector so callers can pass a deep-equality for
object/array selectors.
In `@apps/meteor/app/ui/client/lib/recorderjs/videoRecorder.ts`:
- Line 13: Remove the unused instance field recordingAvailable and all writes to
it: delete the private recordingAvailable declaration, remove the guard that
checks recordingAvailable before assigning true (the idempotent assignment can
remain or be removed), and remove the reset assignment (where recordingAvailable
is set back to false). Ensure no other code reads recordingAvailable; if any
remaining reference exists, remove it so the class no longer maintains this
unused state.
In `@apps/meteor/client/startup/streamMessage/autotranslate.ts`:
- Around line 18-22: The dynamic import's .then() unconditionally re-adds the
handler and can race with isEnabled flipping; update the resolved callback to
check the current enabled state before re-adding: after calling
createAutoTranslateMessageStreamHandler(), call the same isEnabled check (or the
current enable flag) and only invoke clientCallbacks.add('streamMessage',
streamMessage, ..., STREAM_HANDLER_ID) if it's still enabled, otherwise ensure
clientCallbacks.remove('streamMessage', STREAM_HANDLER_ID) and skip adding.
Reference createAutoTranslateMessageStreamHandler, clientCallbacks.add/remove,
and STREAM_HANDLER_ID in the change.
- Around line 25-28: The Users.use.subscribe(applyAutoTranslateStreamHandler)
subscription is too broad and re-runs the async import/handler rotation on every
user record change; narrow it by subscribing only to the current user's changes
or remove it if permission changes are already covered by
PermissionsCachedStore.useReady. Update the registration around
applyAutoTranslateStreamHandler so it either replaces Users.use.subscribe(...)
with a userId-scoped watcher (e.g., subscribe/observe for the current user's
record or Users.findOne(currentUserId).observe/subscribe) or delete that line
and rely on settings.observe('AutoTranslate_Enabled') and
PermissionsCachedStore.useReady; keep the enablement checks
(settings.peek('AutoTranslate_Enabled') and hasPermission('auto-translate'))
unchanged so the handler only re-initializes when the current user’s permissions
or the setting actually change.
In
`@apps/meteor/client/views/omnichannel/businessHours/useIsSingleBusinessHours.ts`:
- Around line 3-4: Remove the historical rationale comment above the
businessHourManager reference in useIsSingleBusinessHours.ts: delete the
two-line comment starting with "businessHourManager holds a single behavior
instance..." and its follow-up about useReactiveValue so the file only contains
implementation-relevant comments; leave any necessary code comments intact and
do not add new explanatory history (commit message should hold the context).
In `@apps/meteor/client/views/root/hooks/useIframe.ts`:
- Line 5: The CallbackError alias declared as "type CallbackError = Error & {
error?: string | number; reason?: string; details?: unknown }" is unnecessary
because the handler only checks truthiness of the error; either remove the alias
and use "Error | null | undefined" in the callback signatures, or keep the alias
but update any call sites to pass the extra fields into the callback so they are
consumed; update the relevant function/method signatures (the callback parameter
types in useIframe's handlers where CallbackError is referenced) to the chosen
simpler type or ensure code constructs and forwards the richer object so the
extra properties are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6e2c51-6d50-4798-bfea-ec80ec34a669
📒 Files selected for processing (41)
apps/meteor/app/autotranslate/client/lib/actionButton.tsapps/meteor/app/autotranslate/client/lib/autotranslate.tsapps/meteor/app/lib/client/index.tsapps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/app/livechat/client/lib/stream/queueManager.tsapps/meteor/app/reactions/client/index.tsapps/meteor/app/reactions/client/methods/setReaction.tsapps/meteor/app/slashcommands-join/client/client.tsapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/app/ui/client/lib/recorderjs/AudioEncoder.tsapps/meteor/app/ui/client/lib/recorderjs/videoRecorder.spec.tsapps/meteor/app/ui/client/lib/recorderjs/videoRecorder.tsapps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxapps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsxapps/meteor/client/definitions/IOAuthProvider.tsapps/meteor/client/importPackages.tsapps/meteor/client/lib/chats/flows/processSetReaction.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/client/lib/chats/readStateManager.tsapps/meteor/client/lib/rooms/roomCoordinator.tsxapps/meteor/client/lib/utils/getConfig.tsapps/meteor/client/lib/utils/timeAgo.tsapps/meteor/client/meteor/overrides/oauthRedirectUri.tsapps/meteor/client/meteor/overrides/userAndUsers.tsapps/meteor/client/meteor/startup/absoluteUrl.tsapps/meteor/client/providers/SessionProvider.tsxapps/meteor/client/startup/incomingMessages.tsapps/meteor/client/startup/roles.tsapps/meteor/client/startup/startup.tsapps/meteor/client/startup/streamMessage/autotranslate.tsapps/meteor/client/views/composer/VideoMessageRecorder/VideoMessageRecorder.tsxapps/meteor/client/views/omnichannel/businessHours/useIsSingleBusinessHours.tsapps/meteor/client/views/room/E2EESetup/RoomE2EESetup.tsxapps/meteor/client/views/room/body/hooks/useFileUploadDropTarget.tsapps/meteor/client/views/room/body/hooks/useUnreadMessages.tsapps/meteor/client/views/room/composer/hooks/useMessageComposerIsReadOnly.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/providers/RoomProvider.tsxapps/meteor/client/views/root/IndexRoute.tsxapps/meteor/client/views/root/hooks/useCorsSSLConfig.tsapps/meteor/client/views/root/hooks/useIframe.ts
💤 Files with no reviewable changes (3)
- apps/meteor/app/lib/client/index.ts
- apps/meteor/app/reactions/client/index.ts
- apps/meteor/client/importPackages.ts
| super(); | ||
|
|
||
| const workerPath = Meteor.absoluteUrl('workers/mp3-encoder/index.js'); | ||
| const workerPath = new URL('workers/mp3-encoder/index.js', __meteor_runtime_config__.ROOT_URL).toString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ROOT_URL is consistently normalized with trailing slash elsewhere.
rg -nP -C2 '__meteor_runtime_config__\.ROOT_URL\b'Repository: RocketChat/Rocket.Chat
Length of output: 3570
🏁 Script executed:
#!/bin/bash
# Check configureBoilerplate.ts for ROOT_URL normalization context
sed -n '40,55p' apps/meteor/server/configuration/configureBoilerplate.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Check roomCoordinator.tsx to see if it has similar new URL usage
sed -n '200,215p' apps/meteor/client/lib/rooms/roomCoordinator.tsx | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 554
🏁 Script executed:
#!/bin/bash
# Search for any normalization of ROOT_URL or trailing slash handling patterns
rg -n 'ROOT_URL.*\/$' -A1 -B1Repository: RocketChat/Rocket.Chat
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Verify the file we're reviewing and check proposed fix formatting
cat -n apps/meteor/app/ui/client/lib/recorderjs/AudioEncoder.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Check if roomCoordinator.tsx has similar issue that should be noted
cat -n apps/meteor/client/lib/rooms/roomCoordinator.tsx | sed -n '205,215p'Repository: RocketChat/Rocket.Chat
Length of output: 419
Worker URL is sensitive to ROOT_URL trailing slash.
new URL('workers/mp3-encoder/index.js', base) treats the path as relative and resolves against the base's directory. If __meteor_runtime_config__.ROOT_URL lacks a trailing slash (e.g., https://host/chat instead of https://host/chat/), the last segment is replaced and the worker resolves to https://host/workers/... instead of https://host/chat/workers/.... Codebase evidence shows ROOT_URL is set without trailing-slash normalization, and other code defensively handles this pattern.
Suggested fix
- const workerPath = new URL('workers/mp3-encoder/index.js', __meteor_runtime_config__.ROOT_URL).toString();
+ const rootUrl = __meteor_runtime_config__.ROOT_URL.endsWith('/')
+ ? __meteor_runtime_config__.ROOT_URL
+ : `${__meteor_runtime_config__.ROOT_URL}/`;
+ const workerPath = new URL('workers/mp3-encoder/index.js', rootUrl).toString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const workerPath = new URL('workers/mp3-encoder/index.js', __meteor_runtime_config__.ROOT_URL).toString(); | |
| const rootUrl = __meteor_runtime_config__.ROOT_URL.endsWith('/') | |
| ? __meteor_runtime_config__.ROOT_URL | |
| : `${__meteor_runtime_config__.ROOT_URL}/`; | |
| const workerPath = new URL('workers/mp3-encoder/index.js', rootUrl).toString(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ui/client/lib/recorderjs/AudioEncoder.ts` at line 11, The
worker URL construction in AudioEncoder.ts uses new
URL('workers/mp3-encoder/index.js', __meteor_runtime_config__.ROOT_URL) which
breaks when ROOT_URL lacks a trailing slash; update the logic that computes
workerPath to ensure the base (__meteor_runtime_config__.ROOT_URL) ends with a
'/' before passing it to new URL (or otherwise join the path correctly) so that
the worker resolves under the app subpath (e.g., keep last segment from being
replaced); change the assignment that defines workerPath accordingly (the
variable and its use in AudioEncoder.ts).
Comment on lines
+205
to
+211
| return new URL( | ||
| router.buildRoutePath({ | ||
| name: config.route.name, | ||
| params: routeData, | ||
| }), | ||
| ); | ||
| __meteor_runtime_config__.ROOT_URL, | ||
| ).toString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate baseURI construction and any ROOT_URL_PATH_PREFIX handling.
fd -t f 'baseURI' -e ts -e tsx | xargs -r cat
rg -nP -C3 '__meteor_runtime_config__\.(ROOT_URL|ROOT_URL_PATH_PREFIX)\b'
# Find all remaining callers of roomCoordinator.getURL to assess blast radius.
rg -nP -C2 'roomCoordinator\.getURL\s*\('Repository: RocketChat/Rocket.Chat
Length of output: 12261
🌐 Web query:
Meteor __meteor_runtime_config__.ROOT_URL whether includes ROOT_URL_PATH_PREFIX
💡 Result:
Yes, Meteor's meteor_runtime_config includes ROOT_URL_PATH_PREFIX when the ROOT_URL environment variable has a path component (e.g., ROOT_URL="https://example.com/myapp/"). It is derived as the path part of ROOT_URL and placed in the client-side runtime config for use in asset paths, Cordova apps, and packages like time-sync that access meteor_runtime_config.ROOT_URL_PATH_PREFIX directly (e.g., for routing handlers). Examples from sources show it explicitly in the config object alongside ROOT_URL, meteorRelease, etc. If ROOT_URL has no path, it is typically empty ('').
Citations:
- 1: https://stackoverflow.com/questions/39066878/how-to-get-meteor-runtime-config-variables-from-server
- 2: time-sync server doesn't seem to work Meteor ROOT_URL configuration Meteor-Community-Packages/meteor-timesync#51
- 3: Fix CSP violation for code that sets variable __meteor_runtime_config__ meteor/meteor#10733
- 4: Avoid ROOT_URL and DDP_DEFAULT_CONNECTION_URL to default to localhost meteor/meteor#5973
- 5: https://www.meteor-ent.com
🏁 Script executed:
# Check what router.buildRoutePath actually returns
rg -A5 -B5 'buildRoutePath\s*\(' apps/meteor/client/router/index.tsx
# Check Meteor.absoluteUrl and compare
rg -A3 'Meteor\.absoluteUrl' apps/meteor/client/lib/rooms/roomCoordinator.tsx
# Find the previous implementation at these lines to understand what changed
git log --oneline -n 20 -- apps/meteor/client/lib/rooms/roomCoordinator.tsx
git show HEAD:apps/meteor/client/lib/rooms/roomCoordinator.tsx | sed -n '205,211p'Repository: RocketChat/Rocket.Chat
Length of output: 526
🏁 Script executed:
# Get the actual buildRoutePath implementation
rg -B2 -A15 'buildRoutePath\s*\(' apps/meteor/client/router/index.tsx | head -40
# Check what ROOT_URL actually contains - is it origin only or full URL with path?
# Look at how ROOT_URL is set/used in config
rg -B3 -A3 'ROOT_URL\s*=' apps/meteor/server/configuration/configureBoilerplate.ts
# Get the git history to see what code was replaced
git diff HEAD~5 HEAD -- apps/meteor/client/lib/rooms/roomCoordinator.tsx | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1044
🏁 Script executed:
# Get buildRoutePath implementation from router
rg -B5 -A20 'buildRoutePath\s*=' apps/meteor/client/router/index.tsx
# Check if there's a git history we can access
git log --oneline apps/meteor/client/lib/rooms/roomCoordinator.tsx | head -5
# Try to see the actual buildRoutePath code in full
ast-grep --pattern 'buildRoutePath\s*=\s*$_'Repository: RocketChat/Rocket.Chat
Length of output: 996
🏁 Script executed:
# Check what Meteor.absoluteUrl() returns and compare with ROOT_URL
rg -B5 -A10 'Meteor\.absoluteUrl\.defaultOptions' apps/meteor/server/configuration/configureBoilerplate.ts
# Check the actual encoding of buildRoutePath to understand if it includes basePath
rg -B3 -A15 'encodePath\s*=' apps/meteor/client/router/index.tsx | head -50
# Most importantly: does ROOT_URL in __meteor_runtime_config__ actually include the path or not?
# Check if there's any test or config that shows what ROOT_URL contains
fd -t f '\.test\.' apps/meteor/server/configuration | head -5
rg '__meteor_runtime_config__\.ROOT_URL' apps/meteor/server --type-add 'server:*.ts' -l | head -5Repository: RocketChat/Rocket.Chat
Length of output: 732
🏁 Script executed:
# Check how basePath is used in the router
rg 'basePath' apps/meteor/client/router/index.tsx -B2 -A2
# Check what Meteor.absoluteUrl actually does and what it used to be
rg -B5 -A10 'Meteor\.absoluteUrl\(\)' apps/meteor/client/lib/rooms/roomCoordinator.tsx
# Most importantly: look at git history to see what the code WAS before
git log --all --oneline --follow -- apps/meteor/client/lib/rooms/roomCoordinator.tsx | head -10
git show 01525f2bc6:apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'Repository: RocketChat/Rocket.Chat
Length of output: 1502
🏁 Script executed:
# Show what was there before the recent commit
git show 01525f2bc6^:apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'
# Or check the entire history of changes to that function
git log -p --follow -- apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | grep -A30 "getURL" | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for Meteor.absoluteUrl usage to understand what was there before
rg 'Meteor\.absoluteUrl' apps/meteor/client/lib/rooms/ -B3 -A3
# Check if there are any tests or examples showing what ROOT_URL contains
rg 'ROOT_URL.*https?://' apps/meteor --type ts --type tsx -C2 | head -40
# Check the actual implementation and see if ROOT_URL ever includes path
rg -B10 -A5 'ROOT_URL.*=' apps/meteor/server/configuration/configureBoilerplate.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 847
🏁 Script executed:
# Check commit history for the specific function getURL
git log -p --follow -S 'getURL' -- apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | head -150
# Or search for when it was changed to use URL constructor
git log --all -S 'new URL.*router.buildRoutePath' -- apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | head -20
git show $(git log --all -S 'new URL.*router.buildRoutePath' --format=%H -- apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | head -1):apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'Repository: RocketChat/Rocket.Chat
Length of output: 549
🏁 Script executed:
# Check if the URL constructor code was added recently
git log --all --oneline -10 -- apps/meteor/client/lib/rooms/roomCoordinator.tsx
# Check what the code was in earlier commits
git show HEAD~1:apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'
git show HEAD~2:apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'
# Try to find when Meteor.absoluteUrl was last used in this file
rg -c 'Meteor\.absoluteUrl' apps/meteor/client/lib/rooms/roomCoordinator.tsxRepository: RocketChat/Rocket.Chat
Length of output: 655
🏁 Script executed:
# Check the commit that fixed "Jump to message" URL generation
git show 25a10b65bc:apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | sed -n '200,220p'
# Or just get the diff of that commit
git show 25a10b65bc -- apps/meteor/client/lib/rooms/roomCoordinator.tsx 2>/dev/null | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2041
🏁 Script executed:
# Find where ROOT_URL_PATH_PREFIX is actually set/initialized
rg 'ROOT_URL_PATH_PREFIX' apps/meteor/server --type ts -B3 -A3 | head -60
# Check how the client receives ROOT_URL and ROOT_URL_PATH_PREFIX
rg -B5 -A5 'meteorEnv.*ROOT_URL' apps/meteor/packages/meteor-cookies/cookies.js
# Most importantly: test what ROOT_URL actually contains by checking existing URL handling code
rg 'new URL.*ROOT_URL' apps/meteor --type ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 2682
🏁 Script executed:
# Check what Meteor.absoluteUrl actually does - search for how it's used
rg 'Meteor\.absoluteUrl\(' apps/meteor/client --type ts --type tsx -B2 -A2 | head -80
# Check if ROOT_URL is supposed to be origin-only or include path
rg 'ROOT_URL' apps/meteor/server/configuration/configureBoilerplate.ts -B10 -A10 | head -80
# Check Meteor package documentation or see how absoluteUrl differs
fd -t f 'absoluteUrl' apps/meteor/server -name '*.ts'Repository: RocketChat/Rocket.Chat
Length of output: 1084
🏁 Script executed:
# Check what Meteor.absoluteUrl does - search documentation or code
rg -i 'meteor.*absoluteurl' apps/meteor --type ts | head -20
# Verify that __meteor_runtime_config__.ROOT_URL includes the full path including prefix
cat apps/meteor/server/configuration/configureBoilerplate.ts | sed -n '40,65p'
# Check the critical line: what is stored in __meteor_runtime_config__.ROOT_URL?
rg '__meteor_runtime_config__\.ROOT_URL = ' apps/meteor/server --type ts -B5 -A5Repository: RocketChat/Rocket.Chat
Length of output: 4043
🏁 Script executed:
# Understand the critical issue: what does __meteor_runtime_config__.ROOT_URL contain?
# Check if there's any documentation or test showing this
rg 'ROOT_URL.*https' apps/meteor/server --type ts -B2 -A2 | head -40
# Most importantly: verify that when ROOT_URL = "https://host/chat/", it's stored with the path
cat apps/meteor/server/configuration/configureBoilerplate.ts | grep -A 30 'ROOT_URL = value'
# Check if Meteor.absoluteUrl actually prepends the path correctly
rg 'Meteor.absoluteUrl.*path' apps/meteor/server --type ts -B2 -A2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 526
🏁 Script executed:
# Verify the exact behavior: get the full configureBoilerplate.ts to understand ROOT_URL assignment
cat apps/meteor/server/configuration/configureBoilerplate.ts | head -70
# Check if there are any sub-path deployment tests or examples
find apps/meteor -name '*.test.*' -o -name '*.spec.*' | xargs grep -l 'ROOT_URL.*/' 2>/dev/null | head -5
# Verify: did Meteor.absoluteUrl actually handle the path correctly?
# Look for documentation or evidence
web_search Meteor.absoluteUrl behavior with path prefix in rootUrlRepository: RocketChat/Rocket.Chat
Length of output: 2175
🏁 Script executed:
# Perfect! Now verify: __meteor_runtime_config__.ROOT_URL is set to the FULL value
# But check if Meteor.absoluteUrl correctly handles the path in rootUrl
# Search for how Meteor.absoluteUrl was actually implemented or used
# The key finding: configureBoilerplate sets __meteor_runtime_config__.ROOT_URL = value (full URL with path)
# Compare: old code used Meteor.absoluteUrl, new code uses new URL with ROOT_URL
# Verify this is indeed a regression by checking: does Meteor.absoluteUrl handle rootUrl with path?
# Check the git diff of commit 25a10b65bc to see the EXACT change
git show 25a10b65bc -- apps/meteor/client/lib/rooms/roomCoordinator.tsx | grep -A 20 'return'Repository: RocketChat/Rocket.Chat
Length of output: 618
Sub-path deployments will lose the ROOT_URL path prefix.
router.buildRoutePath() returns paths with a leading /. When the first argument to new URL() is an absolute path, it replaces the base's pathname, keeping only the origin. The code changed from using Meteor.absoluteUrl() (which correctly handles paths in rootUrl) to new URL(), which does not. For instances deployed with a sub-path (e.g., ROOT_URL=https://host/chat/), the returned permalink will be https://host/channel/general instead of https://host/chat/channel/general. This breaks shareable URLs produced by getPermaLink() and any other consumer of roomCoordinator.getURL().
🛠️ Proposed fix: revert to Meteor.absoluteUrl or make the path relative
Option 1 (simplest):
- return new URL(
- router.buildRoutePath({
- name: config.route.name,
- params: routeData,
- }),
- __meteor_runtime_config__.ROOT_URL,
- ).toString();
+ return Meteor.absoluteUrl(
+ router.buildRoutePath({
+ name: config.route.name,
+ params: routeData,
+ }),
+ );Option 2 (using new URL with relative path):
- return new URL(
- router.buildRoutePath({
- name: config.route.name,
- params: routeData,
- }),
- __meteor_runtime_config__.ROOT_URL,
- ).toString();
+ const path = router.buildRoutePath({
+ name: config.route.name,
+ params: routeData,
+ });
+ const base = __meteor_runtime_config__.ROOT_URL.endsWith('/')
+ ? __meteor_runtime_config__.ROOT_URL
+ : `${__meteor_runtime_config__.ROOT_URL}/`;
+ return new URL(path.replace(/^\//, ''), base).toString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new URL( | |
| router.buildRoutePath({ | |
| name: config.route.name, | |
| params: routeData, | |
| }), | |
| ); | |
| __meteor_runtime_config__.ROOT_URL, | |
| ).toString(); | |
| return Meteor.absoluteUrl( | |
| router.buildRoutePath({ | |
| name: config.route.name, | |
| params: routeData, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/rooms/roomCoordinator.tsx` around lines 205 - 211, The
permalink construction loses the ROOT_URL sub-path because
router.buildRoutePath() returns an absolute path and new URL(path,
__meteor_runtime_config__.ROOT_URL) replaces the base pathname; update the code
in roomCoordinator.getURL() (where router.buildRoutePath(...) and
__meteor_runtime_config__.ROOT_URL are used) to either call
Meteor.absoluteUrl(router.buildRoutePath(...)) like before, or pass a relative
path to new URL by stripping the leading '/' from router.buildRoutePath(...) so
the base pathname is preserved; ensure getPermaLink() consumers receive the
corrected URL.
Comment on lines
+8
to
32
| onLoggedIn(() => { | ||
| // Only event I found triggers this is from ephemeral messages | ||
| // Other types of messages come from another stream | ||
| return sdk.stream('notify-user', [`${getUserId()}/message`], (msg: IMessage) => { | ||
| msg.u = msg.u || { username: 'rocket.cat' }; | ||
| msg.private = true; | ||
|
|
||
| return Messages.state.store(msg); | ||
| }); | ||
| return Messages.state.store(msg); | ||
| }); | ||
| }); | ||
|
|
||
| onLoggedIn(() => { | ||
| return sdk.stream('notify-user', [`${getUserId()}/subscriptions-changed`], (_action, sub) => { | ||
| onLoggedIn(() => { | ||
| return sdk.stream('notify-user', [`${getUserId()}/subscriptions-changed`], (_action, sub) => { | ||
| Messages.state.update( | ||
| (record) => record.rid === sub.rid && ('ignored' in sub && sub.ignored ? !sub.ignored.includes(record.u._id) : 'ignored' in record), | ||
| ({ ignored: _, ...record }) => record, | ||
| ); | ||
| if ('ignored' in sub && sub.ignored) { | ||
| Messages.state.update( | ||
| (record) => record.rid === sub.rid && ('ignored' in sub && sub.ignored ? !sub.ignored.includes(record.u._id) : 'ignored' in record), | ||
| ({ ignored: _, ...record }) => record, | ||
| (record) => record.rid === sub.rid && record.t !== 'command' && (sub.ignored?.includes(record.u._id) ?? false), | ||
| (record) => ({ ...record, ignored: true }), | ||
| ); | ||
| if ('ignored' in sub && sub.ignored) { | ||
| Messages.state.update( | ||
| (record) => record.rid === sub.rid && record.t !== 'command' && (sub.ignored?.includes(record.u._id) ?? false), | ||
| (record) => ({ ...record, ignored: true }), | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Return .stop from the onLoggedIn callbacks — sdk.stream(...) returns a subscription object, not a cleanup function.
onLoggedIn only stores the return value as cleanup when typeof ret === 'function' (see apps/meteor/client/lib/loggedIn.ts). Since sdk.stream(...) resolves to an object with a .stop property (as used in apps/meteor/client/providers/ServerProvider.tsx), the current code never unsubscribes on logout/re-login. The old stream for the previous uid is leaked and a fresh one is layered on top at each login.
🛠️ Proposed fix
onLoggedIn(() => {
- // Only event I found triggers this is from ephemeral messages
- // Other types of messages come from another stream
- return sdk.stream('notify-user', [`${getUserId()}/message`], (msg: IMessage) => {
+ return sdk.stream('notify-user', [`${getUserId()}/message`], (msg: IMessage) => {
msg.u = msg.u || { username: 'rocket.cat' };
msg.private = true;
return Messages.state.store(msg);
- });
+ }).stop;
});
onLoggedIn(() => {
- return sdk.stream('notify-user', [`${getUserId()}/subscriptions-changed`], (_action, sub) => {
+ return sdk.stream('notify-user', [`${getUserId()}/subscriptions-changed`], (_action, sub) => {
Messages.state.update(
(record) => record.rid === sub.rid && ('ignored' in sub && sub.ignored ? !sub.ignored.includes(record.u._id) : 'ignored' in record),
({ ignored: _, ...record }) => record,
);
if ('ignored' in sub && sub.ignored) {
Messages.state.update(
(record) => record.rid === sub.rid && record.t !== 'command' && (sub.ignored?.includes(record.u._id) ?? false),
(record) => ({ ...record, ignored: true }),
);
}
- });
+ }).stop;
});Also consider: the inline comments on lines 9–10 could be moved to JSDoc on the handler or dropped entirely — per coding guidelines implementation code comments are discouraged, though the note about ephemeral-only messages is worth preserving somewhere. As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/startup/incomingMessages.ts` around lines 8 - 32, The
onLoggedIn callbacks should return cleanup functions that call the
subscription's .stop method because sdk.stream(...) returns an object with a
.stop property (not a function), so change both usages to capture the return
value (e.g., const sub = sdk.stream(...)) and return a function that invokes
sub.stop() (or sub.stop?.()) so the stream unsubscribes on logout/re-login;
update the first onLoggedIn that calls Messages.state.store and the second that
sets up subscriptions-changed to follow this pattern.
Comment on lines
+43
to
+46
| Accounts.onLogout(() => { | ||
| removeLocalUserData(); | ||
| status = undefined; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate all Accounts.onLogout handlers to reason about registration/firing order.
rg -nP --type=ts --type=tsx -C4 '\bAccounts\.onLogout\s*\(' apps/meteor apps/meteor/ee apps/meteor/client
# Confirm removeLocalUserData clears Users.state and localStorage.
rg -nP -C2 '\bremoveLocalUserData\s*=' apps/meteor/client/lib/userData.ts
# Find the UserProvider logout path that depends on Users.state.get(userId).
rg -nP -C3 "Users\.state\.get|logoutCleanUp|afterLogoutCleanUpCallback" apps/meteor/client/providers/UserProviderRepository: RocketChat/Rocket.Chat
Length of output: 2091
🏁 Script executed:
# First, find and examine startup.ts to confirm it has Accounts.onLogout
cat -n apps/meteor/client/startup/startup.ts | head -60
# Search for Accounts.onLogout across the codebase
rg -nP --type=ts --type=tsx 'Accounts\.onLogout' apps/meteor/client
# Find what 'ee' is in UserProvider
rg -nP -B10 "ee\.on\('logout'" apps/meteor/client/providers/UserProvider/UserProvider.tsx | head -30Repository: RocketChat/Rocket.Chat
Length of output: 3083
🏁 Script executed:
# Search for other places logoutCleanUp might be called
rg -nP 'logoutCleanUp' apps/meteor/client --type=ts --type=tsx
# Check if there are other logout handlers that might preserve Users.state
rg -nP 'Meteor\.logout|onLogout' apps/meteor/client/lib --type=ts --type=tsx
# Look for any comments about logout order or Users.state preservation
rg -nP -C2 'logout.*order|order.*logout|Users\.state.*logout' apps/meteor/client --type=ts --type=tsxRepository: RocketChat/Rocket.Chat
Length of output: 150
🏁 Script executed:
# Search for logoutCleanUp mentions without tsx type
rg -nP 'logoutCleanUp' apps/meteor/client
# Check for Meteor.logout and related handlers
rg -nP 'Meteor\.logout' apps/meteor/client
# Look for comments explaining logout sequence
rg -nP -C3 'onLogout|logout' apps/meteor/client/startup/startup.ts
# Check if there are e2e tests that verify logoutCleanUp is called
fd -e spec.ts -o -e test.ts apps/meteor/tests | xargs rg -l 'logoutCleanUp' 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 1721
🏁 Script executed:
# Read the userAndUsers.ts file to see the full comment and context
cat -n apps/meteor/client/meteor/overrides/userAndUsers.ts | head -40
# Check the useIframeCommands logout flow
cat -n apps/meteor/client/views/root/hooks/useIframeCommands.ts | sed -n '75,95p'Repository: RocketChat/Rocket.Chat
Length of output: 2628
🏁 Script executed:
# Check where ee emitter's 'logout' event is actually subscribed/used
rg -nP "ee\.on\('logout'" apps/meteor/client/providers/UserProvider
# Find all logout entry points to understand if useIframeCommands is the only path
rg -nP -C2 "Meteor\.logout|logout\(\)" apps/meteor/client/providers/UserProvider/UserProvider.tsx
# Check if ee.on('logout') callback is actually critical or just a secondary path
rg -nP "onLogout.*callback|onLogout:" apps/meteor/client/providers/UserProviderRepository: RocketChat/Rocket.Chat
Length of output: 491
Fix the Accounts.onLogout handler registration order to preserve Users.state for UserProvider's logout cleanup.
The startup.ts handler on line 43–46 registers with Accounts.onLogout during client bootstrap, before UserProvider mounts. This handler clears Users.state.replaceAll([]) and localStorage.clear(), firing first in the Meteor callback chain. The UserProvider (which registers its own Accounts.onLogout handler at the component level) then emits on its internal ee emitter, but by that time Users.state.get(userId) returns undefined. The guard at line 42 of UserProvider.tsx (if (!user) return) exits early, silently skipping afterLogoutCleanUpCallback.run(user) and sdk.call('logoutCleanUp', user).
This race condition is documented in the codebase: apps/meteor/client/meteor/overrides/userAndUsers.ts:5 has a TODO stating "Accounts.onLogin/onLogout breaks callers (e.g. UserProvider's logoutCleanUp) that read userIdStore from within other onLogout callbacks." A workaround exists in apps/meteor/client/views/root/hooks/useIframeCommands.ts:81–89, where logout captures the user before calling Meteor.logout() and invokes logoutCleanUp in the callback. However, this only covers iframe-initiated logout, not the main application logout path via UserProvider.logout().
Reorder the handlers or preserve Users.state until all logout callbacks have run to ensure logoutCleanUp executes consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/startup/startup.ts` around lines 43 - 46, The onLogout
handler in startup.ts removes user data immediately and runs before
UserProvider's onLogout, causing Users.state to be cleared before UserProvider
can run afterLogoutCleanUpCallback.run(user) and sdk.call('logoutCleanUp',
user); change the startup onLogout to postpone the destructive work so
component-level handlers run first—e.g. replace the immediate
removeLocalUserData()/status reset with a deferred call (use Meteor.defer or
setTimeout 0) or otherwise register this cleanup after UserProvider mounts; keep
references to Accounts.onLogout in startup.ts, removeLocalUserData,
Users.state.replaceAll, and UserProvider's
afterLogoutCleanUpCallback.run/sdk.call to verify ordering.
Comment on lines
+48
to
+56
| // Session-resume failure (expired stored token on page load): Meteor has already | ||
| // cleared Meteor.loginToken before this module runs, userId stays null, and no | ||
| // Accounts.onLogout callback fires for this scenario. Detect via the combination | ||
| // of missing token and missing uid at module init and clean up residual keys | ||
| // (e.g. E2EE public_key / private_key). Do NOT subscribe to userIdStore for this — | ||
| // the valid-session resume path is async and would clobber a valid token mid-flight. | ||
| if (!userIdStore.getState() && localStorage.getItem('Meteor.loginToken') === null) { | ||
| removeLocalUserData(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show what removeLocalUserData clears (whole localStorage vs. specific keys).
rg -nP -C3 '\bremoveLocalUserData\s*=' apps/meteor/client/lib/userData.ts
# Look for other places that rely on localStorage keys beyond user/E2EE state (to assess blast radius of localStorage.clear()).
rg -nP --type=ts --type=tsx -C1 "localStorage\.(setItem|getItem)\(" apps/meteor/client | head -n 80Repository: RocketChat/Rocket.Chat
Length of output: 284
🏁 Script executed:
# Read the actual startup.ts code to verify the condition and context
cat -n apps/meteor/client/startup/startup.ts | sed -n '40,65p'Repository: RocketChat/Rocket.Chat
Length of output: 901
🏁 Script executed:
# Search for userIdStore and Tracker.autorun initialization timing
rg -n 'userIdStore|Tracker.autorun' apps/meteor/client/lib/userData.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Find all localStorage operations to understand the blast radius
rg -n 'localStorage\.(setItem|getItem|removeItem|clear)' apps/meteor/client --type ts -C1 | grep -E '(setItem|getItem|removeItem|clear)' | head -40Repository: RocketChat/Rocket.Chat
Length of output: 937
🏁 Script executed:
# Check if localStorage.clear() is called anywhere else to assess the pattern
rg -n 'localStorage\.clear\(' apps/meteor/client --type tsRepository: RocketChat/Rocket.Chat
Length of output: 128
🏁 Script executed:
# Find where userIdStore is defined and when it gets set (Tracker.autorun timing)
rg -n 'userIdStore.*=' apps/meteor/client/lib/userData.ts -A3 -B1Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for Tracker.autorun involving userIdStore or userId
rg -n 'Tracker\.autorun' apps/meteor/client/lib/userData.ts -A5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check when the startup.ts module is loaded relative to other async operations
rg -n 'import.*startup' apps/meteor/client --type ts | head -10Repository: RocketChat/Rocket.Chat
Length of output: 244
🏁 Script executed:
# Look for tests that cover this startup.ts condition
rg -n 'removeLocalUserData|Meteor.loginToken.*null' apps/meteor/tests --type tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search broadly for userIdStore definition
rg -n 'userIdStore' apps/meteor/client/lib --type tsRepository: RocketChat/Rocket.Chat
Length of output: 266
🏁 Script executed:
# Search for where userIdStore comes from in startup.ts
rg -n 'import.*userIdStore' apps/meteor/client/startup/startup.tsRepository: RocketChat/Rocket.Chat
Length of output: 111
🏁 Script executed:
# Look for the state/store definition more broadly
rg -n 'export.*userIdStore' apps/meteor/client --type tsRepository: RocketChat/Rocket.Chat
Length of output: 399
🏁 Script executed:
# Examine userData.ts to find where userIdStore is created
head -50 apps/meteor/client/lib/userData.tsRepository: RocketChat/Rocket.Chat
Length of output: 1243
🏁 Script executed:
# Find where userIdStore is SET (setState calls)
rg -n 'userIdStore.*setState|userIdStore.*=' apps/meteor/client --type tsRepository: RocketChat/Rocket.Chat
Length of output: 615
🏁 Script executed:
# Look for the Meteor account/login resume logic that sets userIdStore
rg -n 'Accounts\.(onLogin|resumeLogin)' apps/meteor/client --type ts -A3Repository: RocketChat/Rocket.Chat
Length of output: 2488
🏁 Script executed:
# Check what happens on page load with Meteor.userId()
rg -n 'Meteor\.userId' apps/meteor/client --type ts | head -15Repository: RocketChat/Rocket.Chat
Length of output: 429
🏁 Script executed:
# Look for where the userId flows into userIdStore during session resume
rg -n 'userIdStore.*setState' apps/meteor/client --type ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 460
🏁 Script executed:
# Look at the full context of userAndUsers.ts Tracker.autorun to understand when it fires
cat -n apps/meteor/client/meteor/overrides/userAndUsers.ts | head -40Repository: RocketChat/Rocket.Chat
Length of output: 1922
🏁 Script executed:
# Check what Accounts.connection.userId() returns on fresh visit vs. expired token
rg -n 'Accounts\.connection' apps/meteor/client --type ts | head -10Repository: RocketChat/Rocket.Chat
Length of output: 315
🏁 Script executed:
# Verify if there's any initialization guard that prevents the condition from firing on fresh visits
cat -n apps/meteor/client/startup/startup.ts | sed -n '1,65p'Repository: RocketChat/Rocket.Chat
Length of output: 2331
🏁 Script executed:
# Check if Meteor.loginToken actually exists on fresh visit or first-time load
rg -n 'Meteor\.loginToken' apps/meteor --type ts | head -15Repository: RocketChat/Rocket.Chat
Length of output: 1406
🏁 Script executed:
# Check if there's any initial state tracking or flags that distinguish fresh visit from expired token
rg -n 'localStorage\.setItem\(.*Meteor\.loginToken' apps/meteor/client --type tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Verify the timing: does Accounts.onLogout fire before startup.ts runs, or after?
rg -n 'import.*startup' apps/meteor/client/startup/index.ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 167
🏁 Script executed:
# Check if there's a way Meteor distinguishes expired token from fresh visit
# Look for any validation or guards around Meteor.loginToken
rg -n 'Meteor\.loginToken|Meteor\.loginTokenExpires' apps/meteor/client --type ts -B2 -A2 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 1080
🏁 Script executed:
# Check the order of module imports to see if anything runs before startup.ts
cat -n apps/meteor/client/main.ts | head -15Repository: RocketChat/Rocket.Chat
Length of output: 467
🏁 Script executed:
# Verify what happens on a fresh visit: does Accounts.onLogout fire?
rg -n 'Accounts\.onLogout|Accounts\.onLogin' apps/meteor/client --type ts -B1 -A3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 4341
🏁 Script executed:
# Check what keys removeLocalUserData is trying to clean up (E2EE public_key / private_key)
rg -n 'public_key|private_key' apps/meteor/client --type ts -B1 -A1 | head -30Repository: RocketChat/Rocket.Chat
Length of output: 5294
🏁 Script executed:
# Verify: does Tracker.autorun run synchronously on module import, or async?
# Check if there's any guard to detect fresh visit vs. expired token
rg -n 'e2e\..*Key|e2e\.privateKey|e2e\.publicKey' apps/meteor/client --type tsRepository: RocketChat/Rocket.Chat
Length of output: 3829
Expired-resume cleanup heuristic will fire on fresh visits, indiscriminately wiping all localStorage.
The condition !userIdStore.getState() && localStorage.getItem('Meteor.loginToken') === null is triggered both on expired-token resume and on first-time visits with no prior login. Since Tracker.autorun in userAndUsers.ts (which sets userIdStore) runs synchronously before startup.ts loads, the only distinguishing check is the token absence, making both scenarios identical to this code.
removeLocalUserData() calls localStorage.clear(), which indiscriminately wipes all keys — not just E2EE (public_key/private_key), but also drafts, theme, OAuth secrets, service-worker state, and config values. On a fresh first-time visit, this blanket clear is unnecessary and destructive.
Consider either:
- Narrowing
removeLocalUserData()to only E2EE keys (per the comment), or - Adding a stronger signal (e.g., a session-expiry marker) to distinguish failed resume from fresh visit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/startup/startup.ts` around lines 48 - 56, The current
heuristic in startup.ts uses (!userIdStore.getState() &&
localStorage.getItem('Meteor.loginToken') === null) and calls
removeLocalUserData(), but removeLocalUserData() currently clears all of
localStorage which is destructive on fresh visits; update the fix so that
removeLocalUserData() only removes the specific E2EE keys (e.g., 'public_key',
'private_key') instead of calling localStorage.clear(), or alternatively add a
stronger resume-failure signal (e.g., set/check a session-expiry marker key
before clearing) and only remove E2EE keys when that marker indicates an expired
resume; use the existing symbols userIdStore, Meteor.loginToken, and
removeLocalUserData to locate and implement the change.
ggazzo
added a commit
that referenced
this pull request
Apr 24, 2026
Ported from #40268. The original Tracker.autorun gated user-data sync on watchUserId() + Meteor.status().connected + !Meteor.loggingIn() before fetching. After we kill Meteor's WebSocket below, status().connected stays false forever, so synchronizeUserData never runs and useUserDataSyncReady never flips, leaving useMainReady stuck. Use onLoggedIn for the initial sync + utcOffset update, subscribe to the Users store to emit 'status-changed' on subsequent server-side status changes (delivered via the user stream), and Accounts.onLogout to reset local state. No Tracker, no Meteor.status, no Meteor.loggingIn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Incrementally removes Meteor runtime imports from the client without changing behaviour. 19 small, independently reviewable commits covering:
Reactivity migrations
ReactiveVar→@rocket.chat/emitter+useSyncExternalStorehook) — also fixes a latent bug where the send button's disabled state was never re-evaluated reactively.ReactiveVar+Tracker.nonreactive+Tracker.afterFlush→ plain fields patched viaupdateRoom, newuseRoomHistoryState(rid, selector)hook) — last client-sideReactiveVar.meteor/session+createReactiveSubscriptionFactory→Map+ Emitter with value-equality check) — eliminatesmeteor/sessionfrom the client entirely.Tracker.autorunwithsettings.peekinside (which never registered a dep — quietly broken) →settings.observeso setting changes actually refetch inquiries.Tracker.autorunreplaced with direct Zustand/settings subscriptions; keeps the one-shot "wait for gate, fetch, unsubscribe" semantics.Tracker.nonreactivewrapper was a no-op outside any autorun; removed.Tracker.autorunover a non-reactive closure value; replaced with plain conditional.Method stubs → explicit optimistic updates
Meteor.methodsstub replaced withrunOptimisticSendMessage(message)called explicitly beforesdk.call. Fixed a CI-caught subtlety where the original mutation pattern relied on Meteor cloning stub args; the new version writes an optimistic copy and leaves the outgoing message pristine.Trivial drops
AudioEncoder.ts:Meteor.absoluteUrl(path)→new URL(path, __meteor_runtime_config__.ROOT_URL)(same forroomCoordinator).getConfig.ts:Meteor._localStorage→window.localStoragewith the same try/catch wrapper.slashcommands-join,useIframe,IOAuthProvider: Meteor type-only imports (e.g.,Meteor.Error,Meteor.LoginWithExternalServiceOptions) inlined as local structural types.RoomE2EESetup:Accounts.storageLocationis typed asWindow['localStorage']in project externals and the key being read isn't Accounts-managed, so usewindow.localStoragedirectly.actionButton(autotranslate):Meteor.startup(…)wrapper was a no-op post-bootstrap; removed.TODOs for overrides that leave with DDP
client/meteor/startup/absoluteUrl.ts,client/views/root/hooks/useCorsSSLConfig.ts,client/meteor/overrides/oauthRedirectUri.ts— each only exists to bridge Meteor-specific behaviour and has no life after the webapp/DDP removal. Annotated with TODOs.Behavioural notes
Livechat_guest_pool_max_number_incoming_livechats_displayedchanges (previously broken due to.peekinsideTracker.autorun)./tableflip,/lenny,/gimme,/shrug,/unflip) no longer get an optimistic insert since they were only covered by the removedsendMessagestub; the main composer path still does.Scope / out of scope
Still on Meteor and intentionally left for follow-ups:
client/meteor/login/*,UserProvider,AuthenticationProvider,2fa/*,CustomOAuth,loggedIn, etc.SDKClient,RestApiClient,CachedStore,Presence,e2ee/rocketchat.e2e.ddpOverREST,settings,totpOnCall,userAndUsers,unstoreLoginToken.client/meteor/minimongo/*.watch.ts/createReactiveSubscriptionFactory: still Tracker-based; keeping them until their consumers (hasPermission, etc.) are migrated — refactoring the factory now would silently break reactivity for everyuseReactiveValuecall.Test plan
jest app/ui/client/lib/recorderjs/videoRecorder.spec.ts— 8/8.+:emoji:shortcut.Livechat_guest_pool_max_number_incoming_livechats_displayedchanges.Task: ARCH-2119
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes